Skip to content

CXH-2379: fix grant/revoke idempotency for DDL-based engines - #151

Open
al-conductorone wants to merge 14 commits into
mainfrom
cxh-2379-baton-sql-fix-grant-and-revoke-idempotency-for-ddl-based
Open

CXH-2379: fix grant/revoke idempotency for DDL-based engines#151
al-conductorone wants to merge 14 commits into
mainfrom
cxh-2379-baton-sql-fix-grant-and-revoke-idempotency-for-ddl-based

Conversation

@al-conductorone

Copy link
Copy Markdown
Contributor

Repeat grant or revoke requests against DDL-based databases (such as Db2) no longer fail; the connector now recognizes when access is already in the requested state and reports the operation as a successful no-op.

Validation-query "no rows" now wraps ErrQueryAffectedZeroRows so the
provisioning layer's errors.Is check reports GrantAlreadyExists /
GrantAlreadyRevoked instead of failing the task. DDL dialects (e.g. Db2)
whose GRANT/REVOKE raise an error rather than affecting rows can only
signal prior state through validation_queries, which previously landed on
the failing path.

Adds regression tests driving Grant/Revoke end-to-end over in-memory
sqlite for both the already-applied (idempotent) and apply cases.
@linear-code

linear-code Bot commented Sep 2, 2026

Copy link
Copy Markdown

CXH-2379

Comment thread pkg/bsql/query.go Outdated
Comment thread pkg/bsql/query.go Outdated
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: CXH-2379: fix grant/revoke idempotency for DDL-based engines

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 2963cce98f5b.
Review mode: incremental since 7cf7eb01
View review run

Review Summary

Scanned the full PR diff for security and correctness, plus the incremental diff for suggestion-level review; the incremental artifact reported no dropped or truncated paths, and go.mod/go.sum are unchanged (all new imports — mysql, pgconn, grpc/status — are already direct dependencies). The new commit adds an ErrValidationNoRows sentinel so the revoke path can tell a no-rows validation short-circuit apart from the revoke queries themselves affecting zero rows, and correctly skips the principal-exists probe in that case — this fixes the spurious-ResourceDeleted hazard, and the plumbing through runRevokeQueries is consistent on every return path. Prior feedback is largely addressed: AuthError now takes the DB name and the driver-coverage gap for go-ora/go_ibm_db/MSSQL/HDB is documented in the function comment, though the underlying driver error is still dropped rather than wrapped into the Unauthenticated status.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • pkg/bsql/query.go:464-467 — the RunRevokeProvisioning doc contract ("still commits and probes ... so retried revokes still surface the deletion") is now stale for DDL engines using validation_queries; the new !fromValidation guard at line 496 skips the probe on that path.
  • pkg/bsql/query.go:672-677 — the DDL branch returns a bare ErrValidationNoRows with no query context and no log line, while the non-DDL branch gained %q; the silent-swallow hazard documented in docs/provisioning.md leaves nothing diagnosable in the logs on Db2/Oracle.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/bsql/query.go`:
- Around line 464-467: The RunRevokeProvisioning doc comment states that when every
  revoke query affects zero rows the function "still commits and probes, then returns
  ErrQueryAffectedZeroRows ... combined with ResourceDeleted when the principal is also
  gone, so retried revokes still surface the deletion." The new !fromValidation guard
  at line 496 makes this false on DDL engines (Db2/Oracle) whose revoke config uses
  validation_queries: the probe is skipped entirely, so a cascaded principal deletion is
  never reported on a retried revoke. Update the doc block to state that the probe runs
  only when the zero-rows result came from the revoke queries executing, not from a
  validation short-circuit, and note the consequence for ResourceDeleted. Apply the same
  clarification to the comment at pkg/bsql/provisioning.go:156-157, which still says
  "The exists-check still runs when the revoke queries affected zero rows".

- Around line 672-677: In runValidationQueries the two no-rows branches are asymmetric.
  The non-DDL branch returns fmt.Errorf with %q and the offending query, but the DDL
  branch returns the bare ErrValidationNoRows sentinel and logs nothing, so on Db2/Oracle
  the failure mode that docs/provisioning.md warns about (a missing or mistyped
  principal_id makes the validation query return no rows and the operation is reported to
  ConductorOne as GrantAlreadyExists / GrantAlreadyRevoked) leaves no diagnostic trace.
  Either add a warn log before the return, e.g. l.Warn with the prepared query as a
  "query" field and a message like "validation query returned no rows; treating as
  idempotent success", or wrap the query into the sentinel with fmt.Errorf using
  %q for the query and %w for ErrValidationNoRows. Both
  errors.Is(err, ErrValidationNoRows) in runRevokeQueries and
  errors.Is(err, ErrQueryAffectedZeroRows) in Grant/Revoke keep working with the wrapped
  form, so no caller changes are needed.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

A validation query returning no rows now maps to ErrQueryAffectedZeroRows
(reported as GrantAlreadyExists / GrantAlreadyRevoked) only on DDL-based
engines (Db2), which don't report rows-affected. Other engines keep using
validation queries as existence preconditions that fail loudly, so a grant
against a missing user or role is no longer silently reported as success.
This also restores the grant_replace abort behavior on those engines: a
replaced-grant revoke whose validation returns no rows returns a plain
error instead of the sentinel, so GrantReplaced is not emitted.

Document the engine-specific ValidationQueries semantics and add a test
covering the non-DDL loud-failure path.
Comment thread pkg/bsql/provisioning_validation_idempotency_test.go

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Mirror TestGrant_ValidationNoRowsOnNonDDLEngineFailsLoudly on the revoke
path: on a non-DB2 engine, a revoke validation query returning no rows is
a failed precondition, so Revoke returns an error with nil annotations
rather than GrantAlreadyRevoked. Pins the false branch of
validationNoRowsMeansIdempotent() for RunProvisioningQueriesWithExecutor.
Comment thread pkg/bsql/query.go Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Comment thread pkg/bsql/query.go
Comment thread pkg/bsql/query.go
// don't report rows-affected, so the validation query is the only zero-effect signal
// available. Engines that report rows-affected keep using validation queries as
// existence preconditions that fail loudly.
func (s *SQLSyncer) validationNoRowsMeansIdempotent() bool {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Review] why only DB2?

The bug report and this PR's own description frame this as a general "DDL-based engines" problem, not a DB2-only thing — and examples/oracle-test.yml has the exact same DDL-shaped GRANT ... TO / REVOKE pattern, so Oracle is probably exposed to the same bug. Hardcoding database.DB2 here means it's still broken there.

Could totally be intentional (only fix what's actually verified, per the stability-first vibe of this repo) — if so no action needed, just curious if that's the reasoning or if it's worth a quick follow-up ticket for Oracle/MSSQL/HDB/Vertica too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed: Oracle got added (case database.DB2, database.Oracle: in validationNoRowsMeansIdempotent()), so this thread's question is answered — but it lands on exactly the blocking concern @mateoHernandez123 raised separately: Oracle has no build tag (pkg/database/oracle/*.go is plain package oracle, unlike Db2's //go:build db2), so it ships in every default binary, and there's no config-level opt-in flag anywhere in pkg/bsql/config.go. That means every existing Oracle deployment using validation_queries" as an existence precondition (the same pattern this repo's own postgres-test.ymldemonstrates as normal) now silently getsGrantAlreadyExists/GrantAlreadyRevoked` on a missing or mistyped principal instead of a loud error — with zero opt-in. Db2 could take this unconditionally because it's opt-in behind a build tag; Oracle can't. This still looks unresolved and blocking to me.

Comment thread pkg/bsql/query.go
- Extract shared runValidationQueries helper so the grant and revoke
  validation loops stop drifting (the copies had diverged on result.Close).
- Warn in the ValidationQueries doc comment that DDL-engine authors must not
  reuse validation_queries as an existence precondition, since a no-rows result
  is reported as idempotent success and would mask real failures.
- Preserve annotations returned by RunGrantProvisioning in the already-exists
  branch so a GrantReplaced from a committed grant_replace revoke survives.
Comment thread pkg/bsql/provisioning.go Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

On the transactional grant path, RunGrantProvisioning returns the
zero-rows sentinel before commit, so the deferred rollback undoes any
grant_replace revoke. Grant reused those annotations, reporting
GrantReplaced for a removal the database no longer reflects. Keep the
returned annotations only on the no_transaction path, where the replace
already committed; otherwise return a fresh GrantAlreadyExists.

Adds regression tests for both the rolled-back (no GrantReplaced, old
grant survives) and committed (GrantReplaced, old grant gone) paths.
Comment thread pkg/bsql/provisioning.go Outdated
Comment thread pkg/bsql/query.go
Comment thread pkg/bsql/query.go
Comment thread pkg/bsql/config.go

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

On Db2 a grant_replace revoke whose validation query returns no rows swallows
ErrQueryAffectedZeroRows and still reports GrantReplaced: the old grant is
already gone, which is the state a replace aims for. Document this at the guard,
cover it with a DB2 test, and add a validation_queries section to docs/db2.md
warning against using them as existence preconditions on Db2.
Comment thread pkg/bsql/query.go Outdated
Comment thread docs/db2.md Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Oracle GRANT/REVOKE are DDL like Db2: an already-applied REVOKE raises
ORA-01951 (and re-GRANT succeeds without affecting rows), so the
validation query is the only zero-effect signal. Add Oracle to
validationNoRowsMeansIdempotent() so validation 'no rows' maps to
GrantAlreadyExists / GrantAlreadyRevoked instead of failing the task.

Verified live against Oracle XE 21c (grant/re-grant -> GrantAlreadyExists,
revoke/re-revoke -> GrantAlreadyRevoked, no ORA-01951, revoke DDL skipped).
Adds an engine-gate regression test and updates the ValidationQueries doc.
Comment thread pkg/bsql/query.go
Comment thread pkg/bsql/query.go Outdated
Comment thread docs/db2.md Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

Replace os.Exit(1) in main.go with exit.LogExit(err) so an auth failure exits
with the mapped gRPC status code instead of a bare 1, letting the CI sync-test
auth-error check actually assert.
Address PR review on the Oracle idempotency change:

- Fix the validationNoRowsMeansIdempotent doc comment. Re-GRANT on Oracle
  succeeds silently; ORA-01951 is a REVOKE-only error, so the old wording
  claiming a repeat GRANT raises ORA-01951 was wrong.
- Add docs/provisioning.md, an engine-neutral home for the no-rows-means-
  idempotent behavior covering both DDL engines (Db2 and Oracle), so Oracle
  operators can find the existence-precondition warning.
- Trim the Db2-scoped section in docs/db2.md to point at the shared doc and
  drop the now-stale "different meaning than every other (pure-Go) engine"
  claim, since Oracle (also pure-Go) now shares the behavior.
Comment thread pkg/bsql/query.go Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Comment thread .github/workflows/ci.yaml
Comment thread pkg/bsql/query.go
Comment thread docs/provisioning.md

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

Pairs with the exit.LogExit change: Validate wrapped the ping error plainly, so
exit mapped auth failures to Unknown(2). database.AuthError maps SQLSTATE class 28
(Postgres/Redshift/Vertica/etc.) and MySQL 1045 to codes.Unauthenticated.
Comment thread pkg/connector/connector.go Outdated
Comment thread pkg/database/autherror.go

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

…evoke

On a DDL engine a revoke whose validation_queries return no rows short-
circuits before any revoke runs. RunRevokeProvisioning still ran the
principal_exists_check probe, so a mistyped principal_id (validation and
probe both empty) falsely reported a still-present principal as deleted,
contradicting the PrincipalExistsCheck contract.

Add a distinct ErrValidationNoRows sentinel (wrapping ErrQueryAffectedZeroRows
so idempotency reporting is unchanged) and skip the exists probe when the
zero-rows result came from validation rather than the revoke queries running.

Also: reword the grant_replace zero-rows comment to cover both sentinel
sources (not just DDL), include the failing query in the loud validation
error, and link docs/provisioning.md from README. Adds a regression test.
AuthError now takes the failing database name so a multi-DB config shows
which handle rejected the credentials, and its doc comment records that
coverage is limited to SQLState-reporting drivers plus MySQL (Oracle/Db2/
MSSQL/HDB fall through to a generic ping error, not Unauthenticated).
@al-conductorone
al-conductorone dismissed stale reviews from github-actions[bot], github-actions[bot], github-actions[bot], and github-actions[bot] September 3, 2026 21:55

Addressed across commits a439489..8c0917e: idempotency gated to Db2+Oracle only (Oracle verified live), GrantReplaced-on-rollback fixed, principal-exists probe skipped on validation-sourced no-rows, comment/doc accuracy fixes, docs/provisioning.md + README link, DB-name in auth error + driver-coverage note. Oracle inclusion is intended and documented.

Comment thread pkg/bsql/query.go
// Skip the probe when the zero-rows came from a validation query (DDL engines): no
// revoke ran, so a no-rows exists-check would falsely report the principal deleted
// "as a side effect of the revoke" when it may still be present.
if existsCheck != nil && !fromValidation {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: the guard is right, but the RunRevokeProvisioning doc comment above (lines 464-467) still promises the function "still commits and probes ... combined with ResourceDeleted when the principal is also gone, so retried revokes still surface the deletion." That contract no longer holds on Db2/Oracle configs that use validation_queries — a retried revoke short-circuits at validation and never reports a cascaded principal deletion. Please update that doc block (and the matching note at pkg/bsql/provisioning.go:156-157) so the trade-off is visible from the contract. (medium confidence)

Comment thread pkg/bsql/query.go
Comment on lines 672 to 677
if !valid {
return fmt.Errorf("validation query returned no rows")
if s.validationNoRowsMeansIdempotent() {
return ErrValidationNoRows
}
return fmt.Errorf("validation query %q returned no rows", q)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: the two branches are now asymmetric in diagnosability. The non-DDL branch gained %q with the offending query, but the DDL branch returns a bare ErrValidationNoRows and emits no log at all — so on Db2/Oracle the exact hazard the new docs/provisioning.md warns about (a mistyped principal_id making a validation query return no rows, reported to C1 as GrantAlreadyExists/GrantAlreadyRevoked) leaves nothing in the logs to diagnose it. Consider a l.Warn("validation query returned no rows; treating as idempotent success", zap.String("query", q)) before the return, or wrapping the query into the sentinel with fmt.Errorf("validation query %q returned no rows: %w", q, ErrValidationNoRows)errors.Is against both sentinels keeps working either way. (medium confidence)

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants